Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #251: segfault (stopClient) and showing error (not active window) #252

Merged
merged 4 commits into from
Sep 21, 2024
Merged

Fixes #251: segfault (stopClient) and showing error (not active window) #252

merged 4 commits into from
Sep 21, 2024

Conversation

rNoz
Copy link
Contributor

@rNoz rNoz commented May 25, 2024 •
edited
Loading

First, thanks for the application :)

This PR fixes these two errors, reported here #251
It also provides some improvements related to error reporting.

  1. Segfault.

Debugged and found.

(gdb) bt
#0  0x0000fffff6450cb4 in QObject::deleteLater() () from /lib64/libQt6Core.so.6
#1  0x000000000056f234 [PAC] in MainWindow::stopClient (this=0xffffffffdb90) at /home/user/spotify-qt/src/mainwindow.cpp:597
#2  0x000000000056f128 in MainWindow::startClient (this=0xffffffffdb90) at /home/user/spotify-qt/src/mainwindow.cpp:584
#3  0x00000000004ff3e8 in SettingsPage::Spotify::restartClient (this=0x126c380) at /home/user/spotify-qt/src/settingspage/spotify.cpp:173
#4  0x00000000005048f8 in QtPrivate::FunctorCall<QtPrivate::IndexesList<0>, QtPrivate::List<bool>, void, void (SettingsPage::Spotify::*)(bool)>::call (
    f=(void (SettingsPage::Spotify::*)(SettingsPage::Spotify * const, bool)) 0x4ff2cc <SettingsPage::Spotify::restartClient(bool)>, o=0x126c380, arg=0xffffffffcd18)
    at /usr/include/qt6/QtCore/qobjectdefs_impl.h:145
#5  0x000000000050422c in QtPrivate::FunctionPointer<void (SettingsPage::Spotify::*)(bool)>::call<QtPrivate::List<bool>, void> (
    f=(void (SettingsPage::Spotify::*)(SettingsPage::Spotify * const, bool)) 0x4ff2cc <SettingsPage::Spotify::restartClient(bool)>, o=0x126c380, arg=0xffffffffcd18)
    at /usr/include/qt6/QtCore/qobjectdefs_impl.h:182
#6  0x0000000000503610 in QtPrivate::QCallableObject<void (SettingsPage::Spotify::*)(bool), QtPrivate::List<bool>, void>::impl (which=1, this_=0x12638a0, r=0x126c380, a=0xffffffffcd18, ret=0x0)
    at /usr/include/qt6/QtCore/qobjectdefs_impl.h:553
#7  0x0000fffff64623c0 in void doActivate<false>(QObject*, int, void**) () from /lib64/libQt6Core.so.6
#8  0x0000fffff7a612b4 [PAC] in QAbstractButton::clicked(bool) () from /lib64/libQt6Widgets.so.6
#9  0x0000fffff7a6159c [PAC] in QAbstractButtonPrivate::emitClicked() () from /lib64/libQt6Widgets.so.6
#10 0x0000fffff7a62e2c [PAC] in QAbstractButtonPrivate::click() () from /lib64/libQt6Widgets.so.6
#11 0x0000fffff7a63028 [PAC] in QAbstractButton::mouseReleaseEvent(QMouseEvent*) () from /lib64/libQt6Widgets.so.6
#12 0x0000fffff799eb2c [PAC] in QWidget::event(QEvent*) () from /lib64/libQt6Widgets.so.6
#13 0x0000fffff793e9f8 [PAC] in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /lib64/libQt6Widgets.so.6
#14 0x0000fffff7948270 [PAC] in QApplication::notify(QObject*, QEvent*) () from /lib64/libQt6Widgets.so.6
#15 0x0000fffff63f20b8 [PAC] in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /lib64/libQt6Core.so.6
#16 0x0000fffff7947558 [PAC] in QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool, bool) () from /lib64/libQt6Widgets.so.6
#17 0x0000fffff79b25e0 [PAC] in QWidgetWindow::handleMouseEvent(QMouseEvent*) () from /lib64/libQt6Widgets.so.6
#18 0x0000fffff79b4f3c [PAC] in QWidgetWindow::event(QEvent*) () from /lib64/libQt6Widgets.so.6
#19 0x0000fffff793e9f8 [PAC] in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /lib64/libQt6Widgets.so.6
#20 0x0000fffff63f20b8 [PAC] in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /lib64/libQt6Core.so.6
#21 0x0000fffff6e83b58 [PAC] in QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) () from /lib64/libQt6Gui.so.6
#22 0x0000fffff6ef2544 [PAC] in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /lib64/libQt6Gui.so.6
#23 0x0000fffff7383118 [PAC] in userEventSourceDispatch(_GSource*, int (*)(void*), void*) () from /lib64/libQt6Gui.so.6
#24 0x0000fffff58b09b4 [PAC] in g_main_context_dispatch_unlocked.lto_priv () from /lib64/libglib-2.0.so.0
#25 0x0000fffff59157a4 [PAC] in g_main_context_iterate_unlocked.isra () from /lib64/libglib-2.0.so.0
#26 0x0000fffff58b2084 [PAC] in g_main_context_iteration () from /lib64/libglib-2.0.so.0
#27 0x0000fffff66f1818 [PAC] in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /lib64/libQt6Core.so.6
#28 0x0000fffff63ff9d4 [PAC] in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /lib64/libQt6Core.so.6
#29 0x0000fffff63faf7c [PAC] in QCoreApplication::exec() () from /lib64/libQt6Core.so.6
#30 0x000000000056be04 [PAC] in main (argc=1, argv=0xffffffffe4d8) at /home/user/spotify-qt/src/main.cpp:107
  1. Error messages when in settings panel.

I don't know what is the use case to only show the error messages when the current window (main one) is the active one, as those messages are coming from the settings window. But I have found a problematic behavior with it, reported in the issue. I have spent a bunch of time trying to guess what was the error here (not considering that it was a configuration error, since first it segfaulted due to using a null ptr). If I saw the error "no username", I would have just checked configuration and changed it. So, definitely, there is value in showing errors independently of the active window.

Also, I don't get why we are not showing an error with Bad credentials if we are still not using the KEYCHAIN. So, I have modified it to be able to see this message:

image

Furthermore, as it can be seen, I propose changing the beginning of that message (from "Failed to start Spotify client:" to "Spotify client error:"), as it will make sense in all scenarios. For instance, when spotify client crashes (e.g. myself killing the librespot process):

image

(You can see how I propose changing also the error messages from "Error 1" to "Process crashed").

Furthermore, since we can stop the client on purpose, and to avoid modifying the settings panel logic, I have updated that message to be "Process stopped or crashed", as can be both cases. I am reusing the mainContent variable to detect if we are closing the app to avoid showing error messages (established in event->accept() on closeEvent)

image
Then, clicking in "Stop client":
image

But not errors are shown in a dialog. The same when we close the application window.

kraxarn
kraxarn requested changes Jul 4, 2024
View reviewed changes
Copy link
Owner

@kraxarn kraxarn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for looking into the issue and fixing it! ☺️
Just some small changes and I'm ready to merge it.

src/mainwindow.cpp Show resolved Hide resolved
src/spotifyclient/runner.cpp Outdated Show resolved Hide resolved
@pfr-dev pfr-dev mentioned this pull request Sep 10, 2024
Open
@kraxarn kraxarn merged commit d647b20 into kraxarn:master Sep 21, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kraxarn kraxarn kraxarn requested changes

Assignees
No one assigned
Labels
None yet
Projects
spotify-qt releases
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rNoz @kraxarn